Add mutex to tree move function, fix #1518 #2082

svenseeberg · 2023-02-18T14:21:08Z

Short description

Prevent race conditions in page tree. This is no replacement for database transactions. This would not catch problems where the process dies in the middle of changing a tree structure.

Proposed changes

Use django-db-mutex module to implement a per class mutex that allows only one page tree move operation at a time.
This can be replaced with basically the same functionality within tree beard if it works.
As we have multiple processes in production, the database back end is used for communication. We could also use Redis, which would actually be a little faster. However, the app implements it this way and it should be good enough for testing if we continue to have 500 errors in our logs.
Saving of other data should continue even when the move action fails. The user receives a notification about the failure to move the page.

Side effects

In a few cases users may have to wait a little longer.
If getting the mutex fails, the user as to redo the action.
The mutex timeout / TTL seems to have no effect that I could verify. However this should still be better than the status quo. In a test I added a sleep for 60s in the block that requires the mutex and set the mutex TTL to 5s. No error was raised.

Resolved issues

Fixes: #1518

Pull Request Review Guidelines

codeclimate · 2023-02-18T14:23:41Z

Code Climate has analyzed commit e43e493 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 56.2% (50% is the threshold).

This pull request will bring the total coverage in the repository to 75.2% (0.0% change).

View more on Code Climate.

svenseeberg · 2023-02-18T14:27:54Z

I will improve the user notification and error handling a little before we merge this.

timobrembeck

Thanks, this will hopefully reduce our problems! 🤞

integreat_cms/cms/models/abstract_tree_node.py

Pipfile

integreat_cms/cms/models/abstract_tree_node.py

svenseeberg · 2023-02-19T13:31:10Z

Using pylint 2.16.2 for python 3.9.15
pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.Exception' ?) instead.
************* Module integreat_cms.cms.models.abstract_tree_node
integreat_cms/cms/models/abstract_tree_node.py:297:20: E1101: Instance of 'AbstractTreeNode' has no 'best_translation' member (no-member)
integreat_cms/cms/models/abstract_tree_node.py:304:20: E1101: Instance of 'AbstractTreeNode' has no 'best_translation' member (no-member)

... a few more things need fixing.

integreat_cms/cms/models/abstract_tree_node.py

Pipfile.lock

timobrembeck

🎉

* Use django-db-mutex module to implement a mutex that allows only one page tree move operation at a time. Co-authored-by: Timo Ludwig <[email protected]>

svenseeberg · 2023-02-21T13:47:35Z

@timoludwig @ulliholtgrave lets merge and deploy? I can have a close look at the prod CMS the remaining day.

ulliholtgrave

Fine by me

timobrembeck · 2023-02-21T13:54:52Z

One caveat I just noticed: The pages are also moved when the page form is submitted, so in theory we also need to put the page form into the same mutex? 🤔
But this PR can be merged anyways, it's definitely an improvement even if it doesn't fix #1518 completely!

svenseeberg · 2023-02-21T14:17:17Z

Yes lets merge and continue that later.

svenseeberg requested a review from a team as a code owner February 18, 2023 14:21

svenseeberg force-pushed the bugfix/tree-beard-mutex branch from d53eaf3 to 99237f5 Compare February 18, 2023 14:25

svenseeberg marked this pull request as draft February 18, 2023 14:27

svenseeberg force-pushed the bugfix/tree-beard-mutex branch 7 times, most recently from e9a80c0 to 2f1696d Compare February 18, 2023 17:32

svenseeberg marked this pull request as ready for review February 18, 2023 17:40

timobrembeck requested changes Feb 18, 2023

View reviewed changes

svenseeberg force-pushed the bugfix/tree-beard-mutex branch from 2f1696d to b527a79 Compare February 18, 2023 20:34

timobrembeck requested changes Feb 18, 2023

View reviewed changes

svenseeberg force-pushed the bugfix/tree-beard-mutex branch 2 times, most recently from de8846f to 058dce1 Compare February 19, 2023 13:23

timobrembeck requested changes Feb 19, 2023

View reviewed changes

svenseeberg force-pushed the bugfix/tree-beard-mutex branch 2 times, most recently from b359a71 to 97fe210 Compare February 19, 2023 18:28

svenseeberg changed the title ~~Add mutex to page tree move function, fix #1518~~ Add mutex to tree move function, fix #1518 Feb 19, 2023

svenseeberg force-pushed the bugfix/tree-beard-mutex branch from 97fe210 to 18c4292 Compare February 21, 2023 13:19

timobrembeck approved these changes Feb 21, 2023

View reviewed changes

Add mutex to page tree move function, fix #1518

e43e493

* Use django-db-mutex module to implement a mutex that allows only one page tree move operation at a time. Co-authored-by: Timo Ludwig <[email protected]>

svenseeberg force-pushed the bugfix/tree-beard-mutex branch from 18c4292 to e43e493 Compare February 21, 2023 13:25

ulliholtgrave approved these changes Feb 21, 2023

View reviewed changes

svenseeberg merged commit 30a8e9a into develop Feb 21, 2023

svenseeberg deleted the bugfix/tree-beard-mutex branch February 21, 2023 14:17

timobrembeck mentioned this pull request Feb 21, 2023

Race conditions in tree operations causing Internal Server Errors #1518

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mutex to tree move function, fix #1518 #2082

Add mutex to tree move function, fix #1518 #2082

svenseeberg commented Feb 18, 2023 •

edited

Loading

codeclimate bot commented Feb 18, 2023 •

edited

Loading

svenseeberg commented Feb 18, 2023

timobrembeck left a comment

svenseeberg commented Feb 19, 2023

timobrembeck left a comment

svenseeberg commented Feb 21, 2023

ulliholtgrave left a comment

timobrembeck commented Feb 21, 2023 •

edited

Loading

svenseeberg commented Feb 21, 2023

Add mutex to tree move function, fix #1518 #2082

Add mutex to tree move function, fix #1518 #2082

Conversation

svenseeberg commented Feb 18, 2023 • edited Loading

Short description

Proposed changes

Side effects

Resolved issues

codeclimate bot commented Feb 18, 2023 • edited Loading

svenseeberg commented Feb 18, 2023

timobrembeck left a comment

Choose a reason for hiding this comment

svenseeberg commented Feb 19, 2023

timobrembeck left a comment

Choose a reason for hiding this comment

svenseeberg commented Feb 21, 2023

ulliholtgrave left a comment

Choose a reason for hiding this comment

timobrembeck commented Feb 21, 2023 • edited Loading

svenseeberg commented Feb 21, 2023

svenseeberg commented Feb 18, 2023 •

edited

Loading

codeclimate bot commented Feb 18, 2023 •

edited

Loading

timobrembeck commented Feb 21, 2023 •

edited

Loading